Skip to content

refactor: complete middleware consolidation (phase 3) — remaining entity middleware and inline loads#8176

Merged
DawoudIO merged 10 commits intomasterfrom
copilot/refactor-middleware-consolidation-phase-3
Mar 6, 2026
Merged

refactor: complete middleware consolidation (phase 3) — remaining entity middleware and inline loads#8176
DawoudIO merged 10 commits intomasterfrom
copilot/refactor-middleware-consolidation-phase-3

Conversation

Copy link
Contributor

Copilot AI commented Mar 5, 2026

Completes the middleware consolidation started in #8166/#8169 by refactoring all five remaining middleware classes to extend AbstractEntityMiddleware and eliminating all identified inline entity loads from route handlers.

Middleware refactors

  • FamilyMiddleware / PersonMiddleware — straightforward extends; removes manual MiddlewareInterface boilerplate and legacy withStatus(412, ...) error responses
  • PersonMiddleware gains an optional $routeParamName constructor param (default 'personId') to support routes whose path param is named differently (e.g. {userID})
  • EventsMiddleware — standardises missing-param response to 412 via renderErrorJSON (was a bespoke 400 via renderJSON)
  • PropertyMiddleware — extends AbstractEntityMiddleware; the constructor-injected $type filter is applied inside loadEntity(): if the loaded property's class doesn't match, null is returned and the base class returns a standard 404. No process() override needed.
  • UserMiddleware — extends AbstractEntityMiddleware with a process() override that preserves the auth logic: current-user identity shortcut sets the attribute directly from the in-memory user; non-admin requesting another user's data gets a 401; admin path delegates to parent::process() for the DB lookup + 404 guard.
// UserMiddleware — auth logic preserved, entity load delegated to parent
public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
{
    $loggedInUser = AuthenticationManager::getCurrentUser();
    if ($loggedInUser->getId() == $userId) {
        return $handler->handle($request->withAttribute('user', $loggedInUser)); // shortcut
    }
    if (!$loggedInUser->isAdmin()) {
        return $response->withStatus(401);
    }
    return parent::process($request, $handler); // loads from DB + 404 guard
}

Inline entity load fixes

Route Was Now
GET /groups/{groupID}/roles GroupQuery::create()->findOneById($groupID) $request->getAttribute('group') + GroupMiddleware
DELETE /groups/{groupID}/removeperson/{userID} PersonQuery::create()->findPk($args['userID']) $request->getAttribute('person') + new PersonMiddleware('userID')
POST /groups/{groupID}/addperson/{userID} same as above same fix
GET /api/user/{userId}/permissions UserQuery::create()->findPk($userId) (redundant — UserMiddleware already on the group) $request->getAttribute('user')

Removes now-unused PersonQuery import from people-groups.php and UserQuery import from user-admin.php.

Original prompt

This section details on the original issue you should resolve

<issue_title>refactor: complete middleware consolidation (phase 3) — remaining entity middleware and inline loads</issue_title>
<issue_description>## Context

PR #8166 introduced AbstractEntityMiddleware, InputSanitizationMiddleware, and RequestParameterValidationMiddleware.
PR #8169 refactored GroupMiddleware, DepositMiddleware, CalendarMiddleware, and KioskDeviceMiddleware to extend AbstractEntityMiddleware, and wired entity middleware to 17 routes.

This issue tracks the remaining work to complete the consolidation.


1 — Middleware files not yet extending AbstractEntityMiddleware

File Route param Attribute Notes
Slim/Middleware/Api/FamilyMiddleware.php familyId family Straightforward refactor
Slim/Middleware/Api/PersonMiddleware.php personId person Straightforward refactor
Slim/Middleware/EventsMiddleware.php id event Returns 400 (not 412) for missing param — needs getNotFoundMessage() + consider moving to Api/
Slim/Middleware/Api/UserMiddleware.php userId user Has additional auth checks alongside entity loading; evaluate separation
Slim/Middleware/Api/PropertyMiddleware.php propertyId property Has constructor-injected type parameter; needs factory pattern or override

Skip: PublicCalendarMiddleware — loads by access token (not route param), sets multiple attributes; not a simple PK load.


2 — Inline entity loads still in route handlers

Route File Line Entity loaded Middleware exists?
GET /{groupID}/roles api/routes/people/people-groups.php ~197 Group via GroupQuery::create()->findOneById($groupID) Yes — GroupMiddleware
DELETE /{groupID}/removeperson/{userID} api/routes/people/people-groups.php ~298 Person via PersonQuery::create()->findPk($userID) Yes — PersonMiddleware
POST /{groupID}/addperson/{userID} api/routes/people/people-groups.php ~337 Person via PersonQuery::create()->findPk($userID) Yes — PersonMiddleware
GET /api/user/{userId}/permissions admin/routes/api/user-admin.php ~135 User via UserQuery::create()->findPk($userId) Yes — UserMiddleware

Suggested order

  1. FamilyMiddleware and PersonMiddleware — simple extends, unblock the inline-load fixes
  2. Fix inline loads in people-groups.php (after PersonMiddleware is refactored)
  3. Fix inline load in user-admin.php (after UserMiddleware is evaluated)
  4. EventsMiddleware — confirm 400 vs 412 is intentional, then refactor + consider namespace move
  5. UserMiddleware and PropertyMiddleware — more complex; separate issues if needed</issue_description>

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

…ity middleware and inline loads

Co-authored-by: DawoudIO <554959+DawoudIO@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor remaining middleware to extend AbstractEntityMiddleware refactor: complete middleware consolidation (phase 3) — remaining entity middleware and inline loads Mar 5, 2026
return gettext('Property not found');
}

public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot do we need process

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No — removed in 5b0a0f9. The type check now lives inside loadEntity(): if the loaded property's class doesn't match $this->type, it returns null, which the base process() handles as a standard 404. No override needed.

…ess override)

Co-authored-by: DawoudIO <554959+DawoudIO@users.noreply.github.com>
@DawoudIO
Copy link
Contributor

DawoudIO commented Mar 5, 2026

@copilot revert the revert just ensure the file need process

@DawoudIO DawoudIO marked this pull request as ready for review March 5, 2026 03:39
@DawoudIO DawoudIO requested a review from a team as a code owner March 5, 2026 03:39
@DawoudIO DawoudIO requested review from DAcodedBEAT, DawoudIO, MrClever, bigtigerku, Copilot, grayeul and respencer and removed request for a team March 5, 2026 03:39
…ith process() override

Co-authored-by: DawoudIO <554959+DawoudIO@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 5, 2026

@copilot revert the revert just ensure the file need process

Done in 1e0b525. UserMiddleware now extends AbstractEntityMiddleware again with the process() override retained — auth shortcut for the current user, admin guard (401), then parent::process() for the DB lookup + 404 path.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Completes phase 3 of the middleware consolidation by migrating remaining entity-loading middleware to AbstractEntityMiddleware and removing a few remaining inline ORM entity loads from route handlers, so route code consistently relies on request attributes populated by middleware.

Changes:

  • Refactors FamilyMiddleware, PersonMiddleware, EventsMiddleware, and PropertyMiddleware to extend AbstractEntityMiddleware (standardizing 412/404 JSON error behavior).
  • Eliminates inline entity loads in group and admin user routes by using $request->getAttribute(...) and wiring the appropriate middleware.
  • Adds PersonMiddleware support for alternate route param names via an optional constructor argument.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/api/routes/people/people-groups.php Removes inline GroupQuery/PersonQuery loads and wires GroupMiddleware/PersonMiddleware to affected routes.
src/admin/routes/api/user-admin.php Removes redundant inline UserQuery load and uses the user request attribute already provided by UserMiddleware.
src/ChurchCRM/Slim/Middleware/EventsMiddleware.php Converts event entity middleware to AbstractEntityMiddleware for consistent missing/404 responses.
src/ChurchCRM/Slim/Middleware/Api/PropertyMiddleware.php Converts property entity middleware to AbstractEntityMiddleware and applies type filtering inside loadEntity().
src/ChurchCRM/Slim/Middleware/Api/PersonMiddleware.php Converts person entity middleware to AbstractEntityMiddleware and adds configurable route param name.
src/ChurchCRM/Slim/Middleware/Api/FamilyMiddleware.php Converts family entity middleware to AbstractEntityMiddleware.

}
return SlimUtils::renderSuccessJSON($response);
})->add(GroupMiddleware::class);
})->add(GroupMiddleware::class)->add(new PersonMiddleware('userID'));
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slim middleware execution is LIFO; with ->add(GroupMiddleware::class)->add(new PersonMiddleware('userID')), the PersonMiddleware runs before GroupMiddleware. That means an extra person lookup will occur even when the groupID is missing/invalid. Consider swapping the add order so GroupMiddleware runs first (fail fast) and only then loads the person when the group exists.

Suggested change
})->add(GroupMiddleware::class)->add(new PersonMiddleware('userID'));
})->add(new PersonMiddleware('userID'))->add(GroupMiddleware::class);

Copilot uses AI. Check for mistakes.
->findByGroupId($groupID);
return SlimUtils::renderJSON($response, $members->toArray());
})->add(GroupMiddleware::class);
})->add(GroupMiddleware::class)->add(new PersonMiddleware('userID'));
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slim middleware execution is LIFO; with ->add(GroupMiddleware::class)->add(new PersonMiddleware('userID')), the PersonMiddleware runs before GroupMiddleware. That means an extra person lookup will occur even when the groupID is missing/invalid. Consider swapping the add order so GroupMiddleware runs first (fail fast) and only then loads the person when the group exists.

Suggested change
})->add(GroupMiddleware::class)->add(new PersonMiddleware('userID'));
})->add(new PersonMiddleware('userID'))->add(GroupMiddleware::class);

Copilot uses AI. Check for mistakes.
@DawoudIO DawoudIO requested a review from Copilot March 5, 2026 18:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@DawoudIO DawoudIO added this to the 7.0.2 milestone Mar 5, 2026
@DawoudIO
Copy link
Contributor

DawoudIO commented Mar 6, 2026

Fixed all issues in middleware consolidation phase 3

✅ Photo API endpoints: GET/POST/DELETE working with proper 404 handling
✅ Middleware ordering: Fixed LIFO execution in people-groups.php
✅ Route restructuring: Moved photo endpoints to avoid PersonMiddleware 500 errors
✅ OpenAPI docs: Added comprehensive specs for POST/DELETE photo endpoints

All 15 photo tests passing. Commit: a92bf83

1 similar comment
@DawoudIO
Copy link
Contributor

DawoudIO commented Mar 6, 2026

Fixed all issues in middleware consolidation phase 3

✅ Photo API endpoints: GET/POST/DELETE working with proper 404 handling
✅ Middleware ordering: Fixed LIFO execution in people-groups.php
✅ Route restructuring: Moved photo endpoints to avoid PersonMiddleware 500 errors
✅ OpenAPI docs: Added comprehensive specs for POST/DELETE photo endpoints

All 15 photo tests passing. Commit: a92bf83

DawoudIO added 3 commits March 5, 2026 17:50
- Split person route groups into two separate groups:
  1. First group handles GET /photo and GET /avatar without PersonMiddleware
     - Allows avatar endpoint to return fallback data for invalid person IDs
     - Allows get photo to serve or return 404
  2. Second group handles POST/DELETE photo, GET person, DELETE person, roles with PersonMiddleware
     - Validates person exists via PersonMiddleware before processing

- Fix PersonMiddleware instantiation with new PersonMiddleware()
  - Required for Slim dependency injection instead of class string

- Update test to expect 404 instead of 412 for missing person
  - AbstractEntityMiddleware returns 404 (not 412) for missing entities

- All 21 tests now passing (6 person + 15 photo)
…y test expectations

- Add comprehensive OpenAPI documentation for GET /person/{id}
- Add OpenAPI documentation for DELETE /person/{id}
- Add OpenAPI documentation for POST /person/{id}/addToCart
- Update family test to expect 404 instead of 412 for missing entity

All modified endpoints now have complete, valid OpenAPI specifications per PR #8176.
@DawoudIO DawoudIO merged commit 6253c9a into master Mar 6, 2026
8 checks passed
@DawoudIO DawoudIO deleted the copilot/refactor-middleware-consolidation-phase-3 branch March 6, 2026 15:02
@github-actions github-actions bot mentioned this pull request Mar 6, 2026
DawoudIO pushed a commit that referenced this pull request Mar 6, 2026
This PR updates the locale translation strings extracted from source
code.

## Triggered By
Commit: `6253c9ab1200a04c226e5f7d2a6ac726e354ca4f`
Message: refactor: complete middleware consolidation (phase 3) —
remaining entity middleware and inline loads (#8176)

Completes the middleware consolidation started in #8166/#8169 by
refactoring all five remaining middleware classes to extend
`AbstractEntityMiddleware` and eliminating all identified inline entity
loads from route handlers.

## Middleware refactors

- **`FamilyMiddleware`** / **`PersonMiddleware`** — straightforward
extends; removes manual `MiddlewareInterface` boilerplate and legacy
`withStatus(412, ...)` error responses
- **`PersonMiddleware`** gains an optional `$routeParamName` constructor
param (default `'personId'`) to support routes whose path param is named
differently (e.g. `{userID}`)
- **`EventsMiddleware`** — standardises missing-param response to
**412** via `renderErrorJSON` (was a bespoke 400 via `renderJSON`)
- **`PropertyMiddleware`** — extends `AbstractEntityMiddleware`; the
constructor-injected `$type` filter is applied inside `loadEntity()`: if
the loaded property's class doesn't match, `null` is returned and the
base class returns a standard 404. No `process()` override needed.
- **`UserMiddleware`** — extends `AbstractEntityMiddleware` with a
`process()` override that preserves the auth logic: current-user
identity shortcut sets the attribute directly from the in-memory user;
non-admin requesting another user's data gets a 401; admin path
delegates to `parent::process()` for the DB lookup + 404 guard.

```php
// UserMiddleware — auth logic preserved, entity load delegated to parent
public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
{
    $loggedInUser = AuthenticationManager::getCurrentUser();
    if ($loggedInUser->getId() == $userId) {
        return $handler->handle($request->withAttribute('user', $loggedInUser)); // shortcut
    }
    if (!$loggedInUser->isAdmin()) {
        return $response->withStatus(401);
    }
    return parent::process($request, $handler); // loads from DB + 404 guard
}
```

## Inline entity load fixes

| Route | Was | Now |
|---|---|---|
| `GET /groups/{groupID}/roles` |
`GroupQuery::create()->findOneById($groupID)` |
`$request->getAttribute('group')` + `GroupMiddleware` |
| `DELETE /groups/{groupID}/removeperson/{userID}` |
`PersonQuery::create()->findPk($args['userID'])` |
`$request->getAttribute('person')` + `new PersonMiddleware('userID')` |
| `POST /groups/{groupID}/addperson/{userID}` | same as above | same fix
|
| `GET /api/user/{userId}/permissions` |
`UserQuery::create()->findPk($userId)` (redundant — `UserMiddleware`
already on the group) | `$request->getAttribute('user')` |

Removes now-unused `PersonQuery` import from `people-groups.php` and
`UserQuery` import from `user-admin.php`.

<!-- START COPILOT ORIGINAL PROMPT -->



<details>

<summary>Original prompt</summary>

> 
> ----
> 
> *This section details on the original issue you should resolve*
> 
> <issue_title>refactor: complete middleware consolidation (phase 3) —
remaining entity middleware and inline loads</issue_title>
> <issue_description>## Context
> 
> PR #8166 introduced `AbstractEntityMiddleware`,
`InputSanitizationMiddleware`, and
`RequestParameterValidationMiddleware`.
> PR #8169 refactored `GroupMiddleware`,
`DepositMiddleware`, `CalendarMiddleware`, and `KioskDeviceMiddleware`
to extend `AbstractEntityMiddleware`, and wired entity middleware to 17
routes.
> 
> This issue tracks the **remaining work** to complete the
consolidation.
> 
> ---
> 
> ## 1 — Middleware files not yet extending `AbstractEntityMiddleware`
> 
> | File | Route param | Attribute | Notes |
> |------|-------------|-----------|-------|
> | `Slim/Middleware/Api/FamilyMiddleware.php` | `familyId` | `family` |
Straightforward refactor |
> | `Slim/Middleware/Api/PersonMiddleware.php` | `personId` | `person` |
Straightforward refactor |
> | `Slim/Middleware/EventsMiddleware.php` | `id` | `event` | Returns
**400** (not 412) for missing param — needs `getNotFoundMessage()` +
consider moving to `Api/` |
> | `Slim/Middleware/Api/UserMiddleware.php` | `userId` | `user` | Has
additional auth checks alongside entity loading; evaluate separation |
> | `Slim/Middleware/Api/PropertyMiddleware.php` | `propertyId` |
`property` | Has constructor-injected type parameter; needs factory
pattern or override |
> 
> > **Skip**: `PublicCalendarMiddleware` — loads by access token (not
route param), sets multiple attributes; not a simple PK load.
> 
> ---
> 
> ## 2 — Inline entity loads still in route handlers
> 
> | Route | File | Line | Entity loaded | Middleware exists? |
> |-------|------|------|---------------|--------------------|
> | `GET /{groupID}/roles` | `api/routes/people/people-groups.php` |
~197 | `Group` via `GroupQuery::create()->findOneById($groupID)` | Yes —
`GroupMiddleware` |
> | `DELETE /{groupID}/removeperson/{userID}` |
`api/routes/people/people-groups.php` | ~298 | `Person` via
`PersonQuery::create()->findPk($userID)` | Yes — `PersonMiddleware` |
> | `POST /{groupID}/addperson/{userID}` |
`api/routes/people/people-groups.php` | ~337 | `Person` via
`PersonQuery::create()->findPk($userID)` | Yes — `PersonMiddleware` |
> | `GET /api/user/{userId}/permissions` |
`admin/routes/api/user-admin.php` | ~135 | `User` via
`UserQuery::create()->findPk($userId)` | Yes — `UserMiddleware` |
> 
> ---
> 
> ## Suggested order
> 
> 1. `FamilyMiddleware` and `PersonMiddleware` — simple extends, unblock
the inline-load fixes
> 2. Fix inline loads in `people-groups.php` (after `PersonMiddleware`
is refactored)
> 3. Fix inline load in `user-admin.php` (after `UserMiddleware` is
evaluated)
> 4. `EventsMiddleware` — confirm 400 vs 412 is intentional, then
refactor + consider namespace move
> 5. `UserMiddleware` and `PropertyMiddleware` — more complex; separate
issues if needed</issue_description>
> 
> ## Comments on the Issue (you are @copilot in this section)
> 
> <comments>
> </comments>
> 


</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

- Fixes #8170

<!-- START COPILOT CODING AGENT TIPS -->
---

💬 We'd love your input! Share your thoughts on Copilot coding agent in
our [2 minute survey](https://gh.io/copilot-coding-agent-survey).

---------

Co-authored-by: copilot-swe-agent[bot]
<198982749+Copilot@users.noreply.github.com>
Co-authored-by: DawoudIO <554959+DawoudIO@users.noreply.github.com>
Co-authored-by: George Dawoud <george@dawouds.com>

## Changes
- Updated `locale/terms/messages.po` with new/modified translatable
strings
- Updated JSON locale keys

## Next Steps
- Review the new strings that need translation
- Merge this PR to update the base locale file
- Upload to POEditor for translation if needed

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor: complete middleware consolidation (phase 3) — remaining entity middleware and inline loads

3 participants